Note for base queries the search is implied:
index=index_a
But for subqueries:
index=index_a | join [search index=index_b] the search field is required.

I see there is a pattern here between parsing things as Json and parsing them as XML. I would recommend creating a parser class that will take an arbitrary object of one of the two types and get the corresponding object a bit more declaratively.

class SplunkResponseParser:
    def parse(object: Json | XML )
        ...

to riff on this - create a parser interface that you can call parse on and it returns to you a consistent result. Switch on the return type and create an instance of either a JsonSplunkResponseParser or XmlSplunkResponseParser. This can be done as a factory method on SplunkResponseParser.

This is a refactoring stategy called "replace conditional with polymorphism"

+1 to the interface idea.

I think there are heavy assumptions here about the state of the JSON, do we know why it is the first field in the entry list? I fear that a parallel job run from a different splunk extractor will make a job, and it will end up being the top of the list here. Or is it the case that a search can have multiple Jobs? Why is it the first one that we choose that we care about.

Maybe:

for entry in job_status.get("entry", []):
    Do checking on all entries that some condition is true. Or that one of the entries is the one we are looking for.

I think it is more standard to do something akin to:

attempts = 0
while should_continue:
    should_continue = success_condition AND attempts < MAX_ATTEMPTS
    attempts += 1

in #425 we're discussing adding a library like tenacity to handle things like this as well.

Link to tenacity

You can also replace this with a global variable SPLUNK_STATUS_CHECK_PERIOD_SECONDS.

I would very much prefer this. My mantra is "prefer no magic numbers/constant strings". Ideally, most constants should be extracted into config or something like it, but at the very least make them module level constants until it's clear which ones need to change often.

Try typing the return types. Instead of nesting try: catches, you can return a completion state of True and False in order to hand off the expected state to the over-arching function.

Why turn to string here, maybe have the interface expect a string

Try to seperate the functionality into a Client and have the extract_records

Agreed

Why are these strings, do we need to change the input parameters to just intake strigified integers?

What if the error is a 504, it could be the case that we hit the endpoint often and they would try to rate-limit us.

IMO:

• 401/403/404 - stop trying
• 2xx are mostly ok. Limiting to 200 may be a problem, but it rarely is.
• 3xx should almost never be seen, because they should be handled by the underlying http library and do whatever forwarding is necessary
• 4xx,5xx - log and try again with backoff
• Connection/DNS error - log and try again with backoff

Also, I feel like this indicates there might be a way to make a more pluggable "client" idea once we figure out a unified retry logic. 🤔

We have an attempt to do this in our internal library but have not had the time to introduce it nodestream. Does everything from the status check handling, retrying, error handling, and json safe loading.

This check is not required. Nodestream does this. You should only be getting called this with a non-none object.

Agreed

This can be computed in the constructor - I don't see a clear reason to generate this string constantly.

This function has a lot going on:

• formulates a request body
• makes the request
• parses the response in one of two different possible return types.

For a simpler request, that may be fine, but given the leg work required, its easy to get lost in this function. I'd recommend breaking this down. Make this function tell a "story" while other functions describe the details.

Also, shouldn't this be search/v2/jobs/{search_id}/results or am I looking at the wrong docs?

actually actually, do we want to support a streaming splunk search result from search/v2/jobs/export?

Sorry, more splunk admin/hacker knowledge is being unlocked:

when we say "splunk extractor" we should consider supporting (and being clear about which we're supporting):

• ad-hoc "streaming" queries (time-bound, no job-id required)
• running a "job" and then getting the results (requires creating the job and then getting the results, what this PR covers)
• accessing "scheduled" search results (by schedule name)

to riff on this - create a parser interface that you can call parse on and it returns to you a consistent result. Switch on the return type and create an instance of either a JsonSplunkResponseParser or XmlSplunkResponseParser. This can be done as a factory method on SplunkResponseParser.

This is a refactoring stategy called "replace conditional with polymorphism"

in #425 we're discussing adding a library like tenacity to handle things like this as well.

My IDE flags this as having too much "cognitive complexity" (Aka too many nested branches and loops)

It would be good to refactor chunks of this into smaller functions to reduce the amount of brainpower required to figure out what's going on.

It would be more "pythonic" to say:

typically the community reserves aliases for modules (and almost always lowercase only):

    import pandas as pd
    import numpy as np

We should prefer using responses or pytest-httpx to handle this mocking, they're more careful about rejecting unexpected calls and emulating an actual http call.

in general I would avoid testing internal/private properties and functions to avoid making tests that rely on implementation details.

These tests can be handled by responses and pytest-httpx by having these mock libraries watching for expected auth headers.

why not assert the whole result?

Hamcrest is great IMO for lists, but it just feels ugly for equalities. This is purely me being a hater of hamcrest.